Skip to content

chore: Release 5.4.4#1945

Merged
fadi-george merged 16 commits intomainfrom
rel/5.4.4
Apr 29, 2026
Merged

chore: Release 5.4.4#1945
fadi-george merged 16 commits intomainfrom
rel/5.4.4

Conversation

@onesignal-deploy
Copy link
Copy Markdown
Collaborator

@onesignal-deploy onesignal-deploy commented Apr 28, 2026

Channels: Current

🐛 Bug Fixes

🛠️ Native Dependency Updates

  • Update Android SDK from 5.7.7 to 5.8.0
    • feat: hash PII (email/SMS) in SharedPreferences at rest (#2620)
    • feat: SDK-4176: gate background threading behind remote feature flag (#2595)
    • feat: SDK-4210: Standardize BACKGROUND_THREADING feature flag key naming (#2598)
    • feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh (#2612)
    • bug: catch exception if opening a notification fails (#2508)
    • Fix: a rare NPE from PermissionViewModel (#2504)
    • fix: add retry for notification opened confirmation (#2606)
    • fix: end initialization early if device storage is locked (#2520)
  • Update iOS SDK from 5.5.0 to 5.5.1

@onesignal-deploy onesignal-deploy requested a review from a team as a code owner April 28, 2026 22:12
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, straightforward version bump for release 5.4.4.

Extended reasoning...

Overview

This PR is a single-line version bump in package.json from 5.4.3 to 5.4.4, accompanying the release of v5.4.4. The release bundles previously-merged PRs (#1942 E2E test workflow, #1944 react-native peer dep bump).

Security risks

None. This is purely a metadata version field change with no executable code, dependencies, or configuration semantics affected.

Level of scrutiny

Minimal scrutiny is appropriate. This is a mechanical release-management change that follows the established pattern used for prior releases. The diff is exactly one line.

Other factors

No bugs were flagged by the bug hunting system. The actual feature/fix changes were already reviewed and merged in their respective PRs (#1942, #1944). No outstanding reviewer comments to address.

Comment thread android/build.gradle Outdated
Comment thread package.json
Comment thread .github/workflows/create-release-pr.yml
Comment thread .github/workflows/e2e.yml
Comment thread react-native-onesignal.podspec
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 examples/setup.sh:38-51 — examples/setup.sh was migrated from bun→vp commands at lines 41/48/66/67, but the surrounding comments at lines 43, 53, 60 and the user-visible echo at line 65 still name 'bun' as the active tool. The echo prints 'Registering tarball with bun (refreshes bun.lock integrity hash)...' on every setup run — actively misleading anyone debugging a CI log who greps for 'bun' expecting to find a bun cache to inspect. Nit, doc-only — vp wraps bun under the hood (packageManager: bun@1.3.13) so functional behavior is unchanged, but the literal command names cited in the comments (bun pm pack, bun add, bun remove) no longer exist as call sites in the file.

    Extended reasoning...

    What is stale

    Verified directly from the diff and the current file. The PR converts four commands from bun to vp:

    • Line 41: vp run build (was bun run build)
    • Line 48: vp pm pack (was bun pm pack)
    • Line 66: vp remove react-native-onesignal (was bun remove ...)
    • Line 67: vp add file:../../react-native-onesignal.tgz (was bun add ...)

    …but the surrounding documentation block still names bun by its CLI:

    • Line 43: # bun pm pack honors package.json's "files" field … — describes the command on line 48, which is now vp pm pack.
    • Line 53: # Always go through bun add so bun.lock's integrity hash … — describes line 67, which is now vp add.
    • Line 57: # subsequent bun install that re-resolved this entry …
    • Line 60: # bun remove first because bun verifies the existing integrity hash — describes line 66, which is now vp remove.
    • Line 65: echo "Registering tarball with bun (refreshes bun.lock integrity hash)..." — printed to stdout on every run.

    The refutation is partially right

    A verifier correctly noted that vp wraps the underlying package manager (per AGENTS.md), and examples/demo/package.json declares packageManager: bun@1.3.13. So the integrity-hash mechanics and bun.lock filename references in the comments remain technically accurate — bun is still the package manager doing the work, just behind the vp wrapper. That's why this is a nit, not a real bug.

    Why it's still worth flagging

    1. The echo is genuinely misleading in a CI context. A future operator reading Registering tarball with bun (refreshes bun.lock integrity hash)… in a workflow log will grep the script for bun to debug a stale-tarball or cache issue, and find only this echo — no actual bun invocation. The command they need to inspect is vp, not bun.

    2. The literal command names cited in the comments don't appear at the call site. A reader applying the rule 'comments should match the code they document' would update line 43 to mention vp pm pack (or both names), line 53 to mention vp add, and line 60 to mention vp remove.

    3. Header comment at line 3 (Invoked from a demo dir … via bun run setup``) is not staleexamples/demo/package.json lines 7-8 still use `bun run setup` for `preandroid`/`preios`, so that reference is correct.

    Step-by-step proof

    1. Open examples/setup.sh at this PR's HEAD.
    2. Read line 65: echo "Registering tarball with bun (refreshes bun.lock integrity hash)...". This string is unconditionally written to stdout when setup runs.
    3. Read lines 66-67: vp remove react-native-onesignal / vp add file:../../react-native-onesignal.tgz. The actual commands invoked are vp, not bun.
    4. Run grep -n bun examples/setup.sh mentally: matches at lines 3, 43, 53, 56, 57, 60, 62, 65 — all comments or the echo, no real bun invocations remain in the script body.

    Suggested fix

    Either (a) rephrase to name the wrapper and the underlying tool together, e.g. echo "Registering tarball with vp (which delegates to bun and refreshes bun.lock's integrity hash)...", or (b) just s/bun/vp/g in the comments and echo, since vp is now the only literal command in the file. Option (a) preserves the explanatory value of mentioning bun.lock; option (b) is the smaller diff.

    Severity

    nit — functional behavior is unchanged, the bun.lock filename references remain accurate, and the fix is purely cosmetic. Flagging because the misleading echo will appear in every CI log going forward and the comments were touched-by-proximity in this exact diff hunk.

Comment thread .github/workflows/create-release-pr.yml Outdated
Comment thread examples/demo/package.json
Comment thread .github/workflows/e2e.yml
Comment thread .github/workflows/create-release-pr.yml Outdated
Comment thread examples/demo/src/services/OneSignalApiService.ts Outdated
@fadi-george fadi-george enabled auto-merge (squash) April 29, 2026 23:55
@fadi-george fadi-george merged commit 89f5f9a into main Apr 29, 2026
8 checks passed
@fadi-george fadi-george deleted the rel/5.4.4 branch April 29, 2026 23:56
Comment on lines +196 to +205
(
cd examples/demo
vp run setup
cd ios
pod install
)
echo "✓ Refreshed examples/demo/ios/Podfile.lock"

# Only commit if there are changes
git add -A
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Forward-looking nit: the new "Refresh demo Podfile.lock" step at create-release-pr.yml:189-207 runs pod install (no --repo-update, no preceding wait gate) seconds after the prior "Update iOS SDK version" step bumped the OneSignalXCFramework pin via sed. If a future iOS SDK release is pod trunk push-ed minutes before this workflow runs and the CocoaPods CDN has not yet indexed it, pod install fails with "Unable to find a specification for OneSignalXCFramework (= X.Y.Z)" — but only after the prior Update Android SDK version / Update iOS SDK version / Update sdk version steps have already committed and pushed package.json + podspec + wrapper-version literals to rel/X.Y.Z, leaving the release branch in a half-finished state requiring manual recovery. No immediate impact for 5.4.4 (5.5.1 has long since propagated through trunk) — but the new step is the symmetric iOS counterpart of the wait-for-maven-artifact gate this PR added on the Android side, and it would benefit from a wait-for-pod-trunk step (or equivalent retry loop) for the same reason. Note --repo-update is largely a no-op for CDN-backed trunk on CocoaPods 1.7+, so a wait/retry is the cleaner fix.

Extended reasoning...

What is wrong

The new "Refresh demo Podfile.lock" step at .github/workflows/create-release-pr.yml:189-207 runs pod install immediately after the preceding "Update iOS SDK version" step (lines 130-156) sed-bumps the OneSignalXCFramework pin in react-native-onesignal.podspec. There is no --repo-update, no pod repo update, and no wait-for-pod-trunk gate between the bump and the install. The only validation upstream is the curl -sf GitHub-release-tag check at line 136 — but that endpoint has no opinion on whether the pod has propagated through the CocoaPods CDN.

This is the same class of race the verifier flagged for e2e.yml:60 (the asymmetric Android Maven Central wait vs. no iOS wait), but at a different file (this PR's newly introduced step) and a different (tighter) race window. The previous comment was scoped to e2e.yml; the fix for that file does not automatically cover this one.

Code path that triggers it

  1. Operator triggers create-release-pr.yml with ios_version: X.Y.Z shortly after the iOS SDK team pod trunk push-ed OneSignalXCFramework X.Y.Z.
  2. "Update Android SDK version" step (if applicable) commits and pushes the Android bump to rel/X.Y.Z.
  3. "Update iOS SDK version" step validates the GitHub release tag exists (succeeds — the tag was created at pod trunk push time), seds the podspec to s.dependency 'OneSignalXCFramework', 'X.Y.Z', commits and pushes.
  4. "Update sdk version" step bumps package.json, the wrapper-version literals in RNOneSignal.java and RCTOneSignal.mm, commits and pushes.
  5. "Refresh demo Podfile.lock" step runs vp run setup then pod install against the freshly-sed-ed podspec.
  6. CocoaPods asks the CDN for OneSignalXCFramework/X.Y.Z.podspec.json. If CDN propagation has not completed (the spec was just registered with trunk), CocoaPods errors with Unable to find a specification for OneSignalXCFramework (= X.Y.Z) and pod install exits non-zero. Under set -e the run-block aborts.
  7. git add -A; git diff --staged --quiet && exit 0; git commit ... never runs, so this step exits without committing the lock — but the prior three steps have already pushed package.json + podspec + wrapper-version commits to rel/X.Y.Z. The release branch is now in a half-finished state.

Why nothing prevents it

  • The GitHub-tag check at line 136 (curl -sf .../releases/tags/${VERSION}) validates only that the GitHub release exists — orthogonal to CDN indexing.
  • pod install with no flags uses the CocoaPods 1.7+ CDN by default (see examples/demo/ios/Podfile declares no source). --repo-update is largely a no-op for CDN-backed trunk because there is no local spec repo to refresh — this is why a retry/wait is the substantive fix.
  • The "Refresh demo Podfile.lock" step is new in this PR; no equivalent backstop existed before.
  • The Android side already has a symmetric protection: this PR adds wait-for-maven-artifact in e2e.yml. The iOS side now has the same kind of race exposure in create-release-pr.yml with no analogous gate.

Why this is genuinely a nit (not a blocker) and not a duplicate

  • Duplicate refutation: The synthesis agent has already considered the prior e2e.yml:60 comment — the substance there was about a different file (e2e.yml, on push: rel/**) and a different pod install callsite (inside setup-demo). This bug is about a new step in create-release-pr.yml introduced by this PR. The fix is localized to each callsite; addressing the e2e.yml comment does not automatically harden this step.
  • Proposed fix being partially wrong: The refutation rightly notes that --repo-update is mostly a no-op for CDN-backed trunk. That observation is correct and informs the recommended fix: a wait-for-pod-trunk step (mirroring the new wait-for-maven-artifact step) or a pod search/retry loop is the substantive remedy, not --repo-update per se.
  • Operationally narrow: CocoaPods CDN propagation is typically sub-minute, and OneSignal iOS SDK 5.5.1 has long since propagated, so this PR's 5.4.4 cut will not flake. The race exists for future iOS bumps where the wrapper PR is triggered close to the iOS SDK's pod trunk push.
  • Recoverable: A failure halts before create-pr runs. The operator can re-run; a second attempt succeeds once CDN propagation completes. Not release-blocking — hence nit.

How to fix

Add a wait-for-pod-trunk step before pod install, mirroring the wait-for-maven-artifact step this PR added on the Android side, e.g.:

- name: Resolve OneSignal iOS SDK version
  id: ios-sdk-version
  run: |
    VERSION=$(grep "OneSignalXCFramework" react-native-onesignal.podspec | sed -E "s/.*'([^']+)'.*/\1/")
    echo "version=${VERSION}" >> "$GITHUB_OUTPUT"

- name: Wait for OneSignalXCFramework on CocoaPods trunk
  uses: OneSignal/sdk-shared/.github/actions/wait-for-pod-trunk@main
  with:
    pod: OneSignalXCFramework
    version: ${{ steps.ios-sdk-version.outputs.version }}

If no wait-for-pod-trunk action exists in OneSignal/sdk-shared, an inline retry loop works equally well:

for i in 1 2 3 4 5; do
  if pod install; then break; fi
  echo "pod install failed (attempt $i); waiting 30s for CDN propagation"
  sleep 30
done

Step-by-step proof

Suppose the iOS SDK team pod trunk push-es OneSignalXCFramework 5.6.0 at T+0. The wrapper releaser triggers create-release-pr.yml with ios_version: 5.6.0 at T+45s.

  1. T+45s — "Update iOS SDK version" step runs. curl -sf .../releases/tags/5.6.0 returns 200 (GitHub tag was created at trunk push time). sed rewrites the podspec to '5.6.0'. git commit -m "Update iOS SDK to 5.6.0" && git pushrel/X.Y.Z now has the podspec bump committed.
  2. T+50s — "Update sdk version" step bumps package.json to the new RN version, seds the wrapper-version literals in RNOneSignal.java and RCTOneSignal.mm, commits and pushes.
  3. T+55s — "Refresh demo Podfile.lock" step runs vp run setup (re-packs the SDK tarball with the new wrapper version) then cd ios; pod install.
  4. CocoaPods queries https://cdn.cocoapods.org/all_pods_versions_*.txt and https://cdn.cocoapods.org/Specs/.../OneSignalXCFramework/5.6.0/OneSignalXCFramework.podspec.json. The version-shard file has not finished propagating. CocoaPods reports Unable to find a specification for OneSignalXCFramework (= 5.6.0)``. Exit non-zero.
  5. Under set -e, the run-block aborts. git diff --staged --quiet && exit 0; git commit ... && git push never executes, so the Podfile.lock refresh is not committed. But the prior three steps have already pushed package.json + podspec + wrapper-version literal commits to rel/X.Y.Z.
  6. The release branch is now half-finished: podspec at 5.6.0, wrapper version bumped, but examples/demo/ios/Podfile.lock still pinned to the previous OneSignalXCFramework version. create-pr (the next job) does not run because update_version failed. Operator must either re-trigger the workflow (will succeed after CDN propagation completes) or manually push the missing lock commit.

Severity

nit — narrow, forward-looking race window. CocoaPods trunk indexing is typically fast and 5.5.1 has long since propagated, so no immediate impact for this PR. But the new step is the symmetric iOS counterpart of the wait-for-maven-artifact Android gate this PR introduces, and the same defensive treatment makes the asymmetry intentional rather than accidental.

🔬 also observed by previous_4

Comment on lines 119 to 124

# Update Android SDK version in build.gradle (handles both api '...' and api('...') syntax)
sed -i '' -E "s/(com\.onesignal:OneSignal:)[0-9.]+/\1$VERSION/" android/build.gradle
sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle
echo "✓ Updated android/build.gradle with Android SDK ${VERSION}"

# Only commit if there are changes
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Forward-looking nit: this PR added grep -q post-substitution verification to the wrapper-version sed in Update sdk version (lines 170-173, 178-181), but the parallel Update Android SDK version (line 121, which this PR just touched to broaden [0-9.]+[^"']+) and Update iOS SDK version (line 146) seds still rely on the unverified sed → echo "✓ Updated" → git diff --staged --quiet && exit 0 pattern. BSD sed -i exits 0 on zero matches, so a future reformat of android/build.gradle (e.g. catalog/multi-line) or the podspec (e.g. s.dependency('OneSignalXCFramework', "5.5.1")) would silently no-op while the log lies — and since neither step has a prior mutation, the staged diff guard fires exit 0 cleanly, leaving the release with a bumped package.json but a stale native pin. Current files match losslessly so no immediate impact; mirroring the existing wrapper-version grep -qF pattern to both earlier seds (two lines per step) closes the same class of bug symmetrically.

Extended reasoning...

What is asymmetric

This PR closed a silent-sed-miss gap in the wrapper-version block at .github/workflows/create-release-pr.yml:168-181 by adding post-substitution grep -q checks:

sed -i '' -E "s/(OneSignalWrapper\.setSdkVersion\(\")[0-9]+(\"\))/\1${PADDED_VERSION}\2/" "$ANDROID_FILE"
if ! grep -q "OneSignalWrapper.setSdkVersion(\"${PADDED_VERSION}\")" "$ANDROID_FILE"; then
  echo "::error::Failed to update wrapper version in ${ANDROID_FILE} to ${PADDED_VERSION}"
  exit 1
fi

But the structurally-identical seds in the two earlier steps in the same job — both of which this PR actively touched (broadened the regexes from [0-9.]+ to [^"']+ / [^']+) — got no analogous verification:

  • Line 121 (Update Android SDK version): sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle followed only by an unconditional echo "✓ Updated …" and git add -A; git diff --staged --quiet && exit 0.
  • Line 146 (Update iOS SDK version): sed -i '' "s/s\.dependency 'OneSignalXCFramework', '[^']*'/.../" react-native-onesignal.podspec with the same unverified pattern.

Why the existing guard does not save these

BSD sed -i '' (job runs on macos-latest) exits 0 even when the pattern matches zero lines. The echo "✓ Updated …" is unconditional so the log lies. And critically, unlike the wrapper-version step, neither of these earlier steps has a prior mutation like npm pkg set version. So when the sed misses, the staged diff is genuinely empty and git diff --staged --quiet && exit 0 exits the step cleanly with success status — no commit, no error, no signal that anything went wrong.

Failure mode (Android — most exposed)

  1. Future contributor reformats android/build.gradle — e.g. moves the dep into a libs.versions.toml catalog and references it as api libs.onesignal, or splits the literal across multiple lines.
  2. Operator triggers the workflow with android_version: 5.9.0. The new curl -sf validates that release exists. Good.
  3. sed runs against the reformatted build.gradle. The com.onesignal:OneSignal:[^"']+ pattern matches zero lines. BSD sed -i exits 0.
  4. echo "✓ Updated android/build.gradle with Android SDK 5.9.0" — workflow log claims success.
  5. git add -A; git diff --staged --quiet && exit 0 — staged diff empty, step exits 0.
  6. Workflow proceeds. Update sdk version bumps package.json + wrapper literal. Refresh demo Podfile.lock regenerates the lock against the still-stale gradle line. Final commits pushed.
  7. rel/5.9.0 ships with package.json at 5.9.0 but android/build.gradle still pinning the prior Android SDK — exactly the failure mode the wrapper-version verification was added to prevent.

The iOS branch is structurally more brittle: the regex requires exact s.dependency + space + single-quoted args + comma-space syntax. A future podspec refactor to parens (s.dependency('OneSignalXCFramework', '5.5.1')) or double-quoted version produces zero matches with the same silent-exit-0 outcome. The downstream Refresh demo Podfile.lock step does not catch it either — pod install resolves against the stale podspec and produces a self-consistent lock at the old version.

Why nothing else catches it

  • curl -sf validates the GitHub release tag exists; it has no opinion on whether sed matched.
  • The downstream pod install resolves whatever the podspec says, so a missed iOS sed produces a self-consistent (but stale) lock.
  • e2e on rel/** may pass if the previously-pinned native SDK is still live on Maven Central / CocoaPods trunk.

Step-by-step proof — Android branch

  1. Reformat the Android dep declaration (any of: catalog reference, multi-line interpolation, removed quotes around version) so the literal com.onesignal:OneSignal:5.8.0 no longer appears on a single line in android/build.gradle.
  2. Workflow runs with android_version: 5.9.0.
  3. sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\15.9.0/" android/build.gradle — pattern matches zero lines. BSD sed exits 0.
  4. echo "✓ Updated android/build.gradle with Android SDK 5.9.0" — log says success.
  5. git add -A — nothing new to stage.
  6. git diff --staged --quiet && exit 0 — empty diff, exits 0.
  7. Step status: success. No human-visible signal of the miss.
  8. Subsequent steps run normally and commit/push the bumped package.json and wrapper literal.

How to fix

Mirror the wrapper-version pattern (~4 lines per step):

sed -i '' -E "s/(com\.onesignal:OneSignal:)[^\"']+/\1$VERSION/" android/build.gradle
if ! grep -qF "com.onesignal:OneSignal:${VERSION}" android/build.gradle; then
  echo "::error::Failed to update android/build.gradle to OneSignal SDK ${VERSION}"
  exit 1
fi
echo "✓ Updated android/build.gradle with Android SDK ${VERSION}"

and analogously for the iOS podspec sed. grep -qF (fixed-string) is sufficient since we are matching the exact value just substituted in.

Severity

nit — current pins (Android 5.8.0, iOS 5.5.1) match the regexes losslessly so there is no immediate impact. Worth flagging because (a) this PR has been twice-revised to harden this very script (curl -sf, wrapper-version grep -q), and (b) the parallel gap in the two earlier seds leaves the same class of silent-drift bug open. Mirroring the new pattern symmetrically while it is fresh costs 4 lines per step.

Comment on lines +5 to +8

# Optional: Android Notification Channel ID for the WITH SOUND test notification.
# Create one in your OneSignal dashboard under Settings > Android Notification Categories.
ONESIGNAL_ANDROID_CHANNEL_ID=
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing nit (lines 1–2 aren't in the diff, but this PR adds an adjacent line that pairs cleanly with the correct pattern, so it's a natural fold-in). .env.example:1 documents "Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-…" but .env.example:2 ships ONESIGNAL_APP_ID=your-onesignal-app-id (non-empty), and the fallback at useOneSignal.ts:32 is ONESIGNAL_APP_ID?.trim() || DEFAULT_APP_ID — only triggers on empty/whitespace, not on the literal placeholder. So a developer who runs cp .env.example .env verbatim ends up calling OneSignal.initialize("your-onesignal-app-id") (which OneSignal rejects) instead of getting the documented default. Fix: ship ONESIGNAL_APP_ID= (matching the new ONESIGNAL_ANDROID_CHANNEL_ID= on line 8) so the fallback actually kicks in, or drop the "used if empty" comment. CI is unaffected (setup-demo writes a real APP_ID); only local-dev onboarding bites.

Extended reasoning...

What is wrong

Three pieces of code disagree about whether ONESIGNAL_APP_ID is a "must-be-replaced placeholder" or a "may-be-empty fallback":

  1. examples/demo/.env.example:1# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef. Comment says the fallback kicks in when the value is empty.
  2. examples/demo/.env.example:2ONESIGNAL_APP_ID=your-onesignal-app-id. Non-empty placeholder.
  3. examples/demo/src/hooks/useOneSignal.ts:32return ONESIGNAL_APP_ID?.trim() || DEFAULT_APP_ID;. The || only short-circuits on empty/whitespace, not on the literal placeholder string.

The result: a developer who follows the documented onboarding flow (cp .env.example .env and run) does not get the default UUID — they get the literal string your-onesignal-app-id passed straight into OneSignal.initialize(), which the API rejects.

Addressing the refutation

The refuting verifier argues the file deliberately mixes two conventions: placeholder-must-replace for required values (ONESIGNAL_APP_ID, ONESIGNAL_API_KEY) and empty-with-default for optional values (ONESIGNAL_ANDROID_CHANNEL_ID). That framing is reasonable on its face, but it does not fit ONESIGNAL_APP_ID specifically, because line 1 of this very file explicitly documents that the value has an empty-triggered fallback (used if ONESIGNAL_APP_ID is empty: 77e32082-…). If ONESIGNAL_APP_ID were truly required, that comment would be wrong; if the comment is right, line 2 should be empty. They cannot both be correct, which is the contradiction.

Why this PR is the natural place to fix it

Lines 1–2 are pre-existing — not in the diff. But this PR adds ONESIGNAL_ANDROID_CHANNEL_ID= on line 8, which is the correct defined-but-empty pattern that pairs cleanly with ONESIGNAL_ANDROID_CHANNEL_ID?.trim() || ... at OneSignalApiService.ts:53 (also added by this PR). So the same pattern is now used both ways within the same 8-line file: line 8 does it right, line 2 does not. That makes the line 2 inconsistency more visible than it was before this PR, and a one-character fold-in (ONESIGNAL_APP_ID=) aligns the two.

Impact

  • CI: unaffected. .github/actions/setup-demo/action.yml writes a non-empty ONESIGNAL_APP_ID from a workflow secret, so e2e never reads .env.example verbatim.
  • End users: unaffected. They install via npm; examples/demo is internal.
  • Local-dev onboarding: the documented cp .env.example .env flow does not produce the documented default. New contributors hit OneSignal.initialize rejection and have to debug why "the default is supposed to kick in".

How to fix

Either:

-# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef
-ONESIGNAL_APP_ID=your-onesignal-app-id
+# Default App ID (used if ONESIGNAL_APP_ID is empty): 77e32082-ea27-42e3-a898-c72e141824ef
+ONESIGNAL_APP_ID=

(matches line 8 ONESIGNAL_ANDROID_CHANNEL_ID= pattern), or drop the "used if empty" comment entirely and tell the developer to replace the placeholder.

Step-by-step proof

  1. Developer clones the repo and runs cp examples/demo/.env.example examples/demo/.env (the documented onboarding step).
  2. .env now contains ONESIGNAL_APP_ID=your-onesignal-app-id.
  3. babel.config.js (react-native-dotenv) inlines ONESIGNAL_APP_ID as the literal string "your-onesignal-app-id" at every callsite.
  4. useOneSignal.ts:32 evaluates "your-onesignal-app-id"?.trim() || DEFAULT_APP_ID. .trim() returns "your-onesignal-app-id" (truthy), so || short-circuits and DEFAULT_APP_ID is not used.
  5. OneSignal.initialize("your-onesignal-app-id") runs. OneSignals API rejects the malformed (non-UUID) app ID.
  6. Developer is confused because line 1 of .env.example told them the documented default would be used "if ONESIGNAL_APP_ID is empty" — and from their point of view, they did not set it.

Severity

pre_existing — line 2 is unchanged by this PR, but the PR adds an adjacent line that uses the correct empty-fallback pattern, so this is the natural moment to align the older line. Trivial fix (one character) and only affects local-dev onboarding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants